Skip to content

Conversation

furkanonder
Copy link
Contributor

@furkanonder furkanonder commented Aug 17, 2025

@furkanonder furkanonder added tests Tests in the Lib/test dir skip news 3.15 new features, bugs and security fixes labels Aug 17, 2025
@furkanonder furkanonder requested a review from picnixz August 17, 2025 14:51
@furkanonder furkanonder removed the 3.15 new features, bugs and security fixes label Aug 18, 2025
@furkanonder
Copy link
Contributor Author

@picnixz

I also added a new test case for wrong types from custom serializers. For example when serializers return None (Only bytes and strings are supported as values), dbm fails. The new test ensure proper TypeError/dbm.error exceptions are raised.

@furkanonder furkanonder requested a review from picnixz August 18, 2025 23:09
@@ -399,6 +411,29 @@ def deserializer(data):
self.assertRaises(shelve.ShelveError, shelve.Shelf, {}, **kwargs)
self.assertRaises(shelve.ShelveError, shelve.BsdDbShelf, {}, **kwargs)

def test_custom_serializer_returns_wrong_type_for_key(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test now duplicates the one when serializer returns None. As the key is never serialized, I don't think we need this one anymore.

def serializer(obj, protocol=None):
pass
def serializer(obj, protocol=None):
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pass
return ["value with invalid type"]

How about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test case, our goal is to test the incomplete serializer. However, with the proposed change, we're defining an invalid serializer. I believe it would make more sense to use this change in a new test case, such as def test_custom_invalid_serializer(self):

Copy link
Member

@picnixz picnixz Aug 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but incomplete serializer just returns None which is the same as defining an invalid serializer (as the error is the same in both cases), so I'm not sure there is a notion of being "incomplete" here that we can use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants